Repository navigation
mcpx: paths.config governs the config chain (two-pass, #1842) - #2477
Conversation
Approach C from issue #1842: config.ResolveChainFrom resolves the chain in two passes. Discovery walks the default locations seeded by MCPX_PATHS_CONFIG / --paths-config (global flag captured in main like --config) and reads only paths.config, honouring the null splice; the load pass resolves that list, each file governing the chain farther than itself. A cycle is refused by identity naming the closing path; a paths.config found only in pass 2 is reported via ChainInfo.Divergent and mcpx config --sources (which now shows both passes distinctly), never applied. MCPX_CONFIG bypasses both passes. LoadFrom and the settings file layer both consume the resolved chain. consumed_test allowlist comment and docs (declared-but-unread.md, configuration.md) truthed up. warnPathsConfigFile now warns on entries naming nothing that exists, since file entries mean exactly that file now.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 55 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
TestNoUndeclaredEnvRead caught chain.go reading MCPX_PATHS_CONFIG by name: it seeds the discovery pass before any Set exists, so it belongs on rawReadAllowed with the reason, alongside MCPX_CONFIG. Verified with -count=1 (the guard reads the tree at runtime, so a cached test result can hide this).
|
Follow-up commit e013229: the full-suite run caught a real failure the package runs missed — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcf9151591
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A bootstrap flag like --config: it decides which files | ||
| // the settings are read from, so it is captured here, | ||
| // before any settings exist (issue #1842). | ||
| config.SetPathsConfigSeed(splitList(args[1])) |
There was a problem hiding this comment.
When --paths-config adds a configuration file and a command needs to auto-start a daemon, the seed exists only in this process's flagSeed. Connect keys the expected socket from the expanded configuration, but Client.EnsureDaemon (internal/cli/client.go) forwards only --config, not this seed. Without an equivalent MCPX_PATHS_CONFIG environment variable, the child loads the default chain and listens on a different configuration fingerprint, so the client times out waiting for its socket. Propagate the bootstrap seed to the spawned process as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed forward in PR #5165 (issue #2507, commit 2a88cd5): config.PathsConfigSeedEnv() serializes the flag seed as MCPX_PATHS_CONFIG, and the daemon autostart path in internal/cli/client.go now sets it as the child daemon's environment, so an auto-started daemon resolves the same chain as the invoking process. Covered by TestPathsConfigSeedEnvForwardsFlagSeed (internal/config).
| for _, loc := range expandAll(info.Seeded, wd) { | ||
| add0(loc) | ||
| } | ||
| for _, loc := range SearchPathFrom(wd) { | ||
| add0(loc) |
There was a problem hiding this comment.
For MCPX_PATHS_CONFIG='[null,"/custom/config.json"]', expandAll discards the splice position and this code places the custom file before every default location, giving it higher rather than lower precedence. A seed without null also retains all default locations instead of replacing them. This contradicts the setting's help in internal/settings/registry.go: “Without a null the list replaces outright.” Build the discovery list using the before/after/null result so the seed does not load unintended configurations or reverse precedence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed forward in PR #5165 (issue #2507, commit 2a88cd5): pass 1 of ResolveChainFrom now splices the seed around the default walk via expandEntries — entries before the null land nearer, entries after it farther, and a seed with no null replaces the default walk outright, as the registry help states. Covered by TestPathsConfigSeedNullSplicesAroundDefaults and TestPathsConfigSeedWithoutNullReplacesDefaults.
| // A bootstrap flag like --config: it decides which files | ||
| // the settings are read from, so it is captured here, | ||
| // before any settings exist (issue #1842). | ||
| config.SetPathsConfigSeed(splitList(args[1])) |
There was a problem hiding this comment.
For mcpx --paths-config /a --paths-config /b config, each call replaces flagSeed, so only /b participates in discovery. The setting is declared Repeatable: true in internal/settings/registry.go, and the existing binding implementation accumulates repeated occurrences. The new bootstrap interception bypasses that behavior and silently drops earlier configuration sources. Collect all occurrences before installing the seed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| var doc struct { | ||
| Paths struct { | ||
| Config []any `json:"config"` | ||
| } `json:"paths"` | ||
| Flat []any `json:"paths.config"` |
There was a problem hiding this comment.
An existing configuration such as {"paths":{"config":"./shared"}} now fails discovery with a JSON unmarshal error because these fields require arrays. The normal settings file loader accepts this representation: ApplyFile preserves string values and Set.List parses comma/newline-separated strings as well as JSON arrays. Since chain resolution now runs before command dispatch, a previously accepted configuration prevents commands from running at all. Normalize the discovery value using the same accepted representations as the settings layer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| if inChain0[chainIdentity(loc)] { | ||
| edges[f] = append(edges[f], loc) |
There was a problem hiding this comment.
When discovered files A and B refer to each other through symlink aliases, the membership test recognizes their identities but the graph stores the original path strings. findCycle consequently treats those aliases as unrelated terminal vertices and accepts the cycle instead of returning the promised cycle error. This is reproducible with A declaring a symlink to B and B declaring a symlink to A. Use canonical identities consistently for graph keys, edge targets, and traversal roots, retaining display paths separately for diagnostics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| } | ||
| return -1 | ||
| } | ||
| for _, f := range chain0 { |
There was a problem hiding this comment.
When a nearer file reorders already-discovered configurations, this loop still applies their declarations in the original discovery order. For discovery order A,B,C, let A declare [C,B], B declare [X], and C declare [B], all without null. A moves C before B, but B is processed first and adds X; C then replaces its remainder with B, dropping X even though B remains in the chain. The result is A,C,B rather than A,C,B,X, silently excluding X's servers and settings. Apply declarations in the current load-chain order so a retained file's declaration is not undone by a now-nearer file processed later.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed forward in PR #5165 (issue #2507, commit 2a88cd5): declarations now apply in the resolved chain's order — the build loop rescans the evolving load chain from the nearest end after each application — so a file a nearer list moved up governs before the ones it displaced, and extensions it added are no longer dropped. Covered by TestPathsConfigDeclarationsApplyInChainOrder, which reproduces this finding's A/B/C/X example exactly.
Implements issue #1842 (Approach C, settled by triage task 2736):
paths.confignow governs the rest of the config chain.What changed
config.ResolveChainFrom(pkgs/mcpx/internal/config/chain.go) — two-pass resolution:SearchPathFrom), seeded byMCPX_PATHS_CONFIG/--paths-config, are read forpaths.configonly, honouring the null splice.paths.configgoverns the chain farther than itself; a list with no null replaces the farther remainder.paths.configfound only in pass 2 is reported, not applied (ChainInfo.Divergent, shown bymcpx config --sources).MCPX_CONFIGbypasses both passes.LoadFromand the settings file layer (configFilesFarthestFirst) both consume the resolved chain, so added files contribute settings as well as servers.--paths-configis a global flag captured inmainalongside--config(it decides which files settings are read from, so it must be captured before any settings exist).mcpx config --sourcesshows the discovery pass and load pass distinctly, plus any divergence reports.consumed_test.goallowlist comment forpaths.config,docs/declared-but-unread.md(no longer "left open"), anddocs/configuration.md(two-pass section).warnPathsConfigFilenow warns on entries naming nothing that exists — file entries mean exactly that file now, so the old mcpx daemons: dead records listed forever, custom-state daemons missed, paths.config file entry silently ignored #278 warning no longer applies.Tests
internal/config/chain_test.go: pass-1 extension changes the load set, cycle refused naming the closing path, pass-2 divergence reported and not applied,MCPX_CONFIGbypass, env seeding, no-null replacement.go test ./internal/config/ ./internal/settings/pass;internal/clipasses isolated;go vetclean.go test ./...run under parallel-suite contention showed oneinternal/daemontermination ("signal: terminated", 11s) that did not reproduce in isolated package runs; CI is the arbiter for the full suite.Queue task 3059. Merging remains gated by the merge-queue issue #2403 — do not merge until that clears; a follow-up lane will verify CI and complete the task.
agent-note:v1 — worker lane D, task 3059.
Need help on this PR? Tag
@codesmith-botwith what you need.